fix($http): JSON parse failure #15724

chirag64 · 2017-02-19T10:32:58Z

Fixes #15695

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
#15695

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

Fixes angular#15695

googlebot · 2017-02-19T10:32:59Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2017-02-19T10:32:59Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

chirag64 · 2017-02-19T10:40:01Z

I signed it!

googlebot · 2017-02-19T10:40:03Z

CLAs look good, thanks!

googlebot · 2017-02-19T10:40:03Z

CLAs look good, thanks!

gkalpak · 2017-02-20T08:28:45Z

docs/content/error/$http/baddata.ngdoc

+@fullName Bad JSON Data
+@description
+
+This error occurs when the data parameter passed to the {@link ng.$http `defaultHttpResponseTransform`} service is not a valid JSON object.


Please limit lines to 100 chars.

The link should point tot a specific section (e.g. ng.$httpProvider.defaults, ng.$http.defaults, or ng$http#default-transformations).

BTW, I just noticed that $httpProvider.defaults.transformRequest and $httpProvider.defaults.transformResponse are not documented. They should be.

gkalpak · 2017-02-20T08:52:04Z

docs/content/error/$http/baddata.ngdoc

+@description
+
+This error occurs when the data parameter passed to the {@link ng.$http `defaultHttpResponseTransform`} service is not a valid JSON object.
+`defaultHttpResponseTransform` expects the first parameter as a valid JSON object if the second parameter of headers specifies a Content-Type of JSON.


Don't mention defaultHttpResponseTransform, because it is private and the users don't know what it is.
Just say that "The default transformResponse will try to parse the response as JSON if the Content-Type header is application/json or the response looks like a valid JSON-stringified object or array".

gkalpak · 2017-02-20T08:53:03Z

docs/content/error/$http/baddata.ngdoc

+This error occurs when the data parameter passed to the {@link ng.$http `defaultHttpResponseTransform`} service is not a valid JSON object.
+`defaultHttpResponseTransform` expects the first parameter as a valid JSON object if the second parameter of headers specifies a Content-Type of JSON.
+
+The error message should provide additional context such as the actual value of the parameter that was received.


actual value of the parameter that was received --> actual response

gkalpak · 2017-02-20T08:54:28Z

src/ng/http.js

+        try {
+          data = fromJson(tempData);
+        } catch (e) {
+          throw minErr('$http')('baddata', 'Data must be a valid JSON object. Received: {0}', data);


We already have an $httpMinErr instance. Use that.

Also, please, embed the actual error message into the minErr.

gkalpak · 2017-02-20T08:57:04Z

For tests, you can "train" $httpBackend with an invalid response and call $http (which will indirectly invoke defaultHttpResponseTransform).

Narretz · 2017-02-20T10:41:33Z

@chirag64 There's already a test in httpSpec that should show you how to test this, see my comment here: #15695 (comment)

In short, you don't access the http transform function directly. You call http, and define a mock json answer that will be parsed.

Fixes angular#15695

gkalpak · 2017-02-21T17:32:12Z

docs/content/error/$http/baddata.ngdoc

+@fullName Bad JSON Data
+@description
+
+The default @{link ng$http#default-transformations `transformResponse`} will try to parse the


Shouldn't this be ng.$http#... (with a dot after ng)?

gkalpak · 2017-02-21T17:33:27Z

docs/content/error/$http/baddata.ngdoc

+
+To resolve this error, make sure you pass a valid JSON data object to `transformResponse`.
+
+For more information, see the {@link ng$http#default-transformations `transformResponse`} service


This is not necessary, since we already link to default-transformations above imo.

gkalpak · 2017-02-21T17:34:14Z

docs/content/error/$http/baddata.ngdoc

+
+The error message should provide additional context such as the actual response.
+
+To resolve this error, make sure you pass a valid JSON data object to `transformResponse`.


a valid JSON data object --> valid JSON data

Also, the correct resolution depends on the source of the problem. In some cases, one might need to send correct headers (to not include application/json for example).

How about

To resolve this error, make sure you pass valid JSON data to transformResponse or use the appropriate Content-Type instead of the specified application/json if passing non-JSON data.

How about:

To resolve this error, make sure you pass valid JSON data to transformResponse or use an appropriate Content-Type header for non-JSON data.

gkalpak · 2017-02-21T17:37:03Z

test/ng/httpSpec.js

+        it('should return JSON data with error message if JSON is invalid', function() {
+          var errCallback = jasmine.createSpy('error');
+          $httpBackend.expect('GET', '/url').respond('{abcd}', {'Content-Type': 'application/json'});
+            $http({method: 'GET', url: '/url'}).then(callback).catch(errCallback);


Wrong indentation.

Nit: It is simpler to use $http.get('/url').

gkalpak · 2017-02-21T17:40:26Z

src/ng/http.js

+          data = fromJson(tempData);
+        } catch (e) {
+          throw $httpMinErr('baddata', 'Data must be a valid JSON object. Received: "{0}". ' +
+          'Error occurred: "{1}"', data, e);


Maybe: Error occurred --> Original error or Parse error

…son-parse-fix

Fixes angular#15695

Narretz · 2017-02-22T18:26:12Z

One thing we need to decide is if the original response data should be available in the thrown error, for recovery attempts. I think it's useful, but we don't really have the option for this. the data is only available in serialized form in the error (or is it), so we'd have to create a new API or wrap the error with a different response object.

gkalpak · 2017-02-22T21:08:56Z

@Narretz, not sure what you mean 😕 The original response is usually just text so we can include it in the error message without further serialization.

chirag64 · 2017-03-02T12:14:44Z

@gkalpak Please let me know if you want me to make any changes on top of the last commit that I made.

Narretz · 2017-03-02T16:37:33Z

@gkalpak My concern is that you cannot get the original response string out the minErr without some extra parsing, because the data passed to the minErr is stringified into the message template. So with this solution it's harder to get the actual response string in case you want to something with the response only.

gkalpak · 2017-03-03T11:10:12Z

I see. I am that concerned about that. If they just want to log it (e.g. to the server), the extra context won't hurt. And if they want to try and fix the issue or handle it in some other way, then they having their own tranformResponse sounds a better approach anyway.

But I don't feel too strongly about it.

The problem is that I don't think it is possible to somehow pass multiple values (e.g. our error and the response as searate entities). Unless you have a clever idea 😃

Narretz · 2017-03-03T11:19:44Z

Ok, I think it's reasonable to implement your own transformer if you need recovery or something special.

@chirag64 there's a failing test that expects something else from a failed JSON transform at

angular.js/test/ng/httpSpec.js

Lines 1372 to 1383 in 3dc0096

    
           it('should forward json deserialization errors to the http error handler', 
        
               function() { 
        
             var errCallback = jasmine.createSpy('error'); 
        
             $httpBackend.expect('GET', '/url').respond('abcd', {'Content-Type': 'application/json'}); 
        
             $http({method: 'GET', url: '/url'}).then(callback).catch(errCallback); 
        
             $httpBackend.flush(); 
        
             expect(callback).not.toHaveBeenCalled(); 
        
             expect(errCallback).toHaveBeenCalledOnce(); 
        
             expect(errCallback.calls.mostRecent().args[0]).toEqual(jasmine.any(SyntaxError)); 
        
           });

. Please

remove this test
move your added test at this position (in the section about default response transform).

…son-parse-fix

Fixes angular#15695

chirag64 · 2017-03-04T13:58:58Z

@Narretz done 👍🏼

gkalpak · 2017-03-04T21:38:06Z

test/ng/httpSpec.js

-
-            $httpBackend.expect('GET', '/url').respond('abcd', {'Content-Type': 'application/json'});
-            $http({method: 'GET', url: '/url'}).then(callback).catch(errCallback);
+            $httpBackend.expect('GET', '/url').respond('{abcd}', {'Content-Type': 'application/json'});


Why this change?

I've replaced the it function of forwarding json deserialization errors with mine. The above highlighted statements are pretty much similar to the old code except that we're using an object '{abcd}' within the string instead of a direct string 'abcd'. Are you referring to that string or something else?

That's fine with me. @gkalpak wdyt?

@chirag64, yes, I am referring to the string. Why change abcd to {abcd}?
_{I am mostly curious - I don't think it makes any difference.}

I wanted to make it look clear that we're passing an invalid JSON instead of passing a string.

gkalpak · 2017-03-13T17:20:16Z

LGTM. Leaving it to @Narretz to give the final LGTM.

Narretz · 2017-03-14T10:17:43Z

LGTM, please merge @gkalpak

Fixes #15695 Closes #15724

Fixes angular#15695 Closes angular#15724

fix($http): JSON parse failure

68ced16

Fixes angular#15695

googlebot added the cla: no label Feb 19, 2017

googlebot added cla: yes and removed cla: no labels Feb 19, 2017

gkalpak added component: $http frequency: low severity: confusing type: bug labels Feb 20, 2017

gkalpak added this to the Backlog milestone Feb 20, 2017

gkalpak suggested changes Feb 20, 2017

View reviewed changes

gkalpak added needs: test needs: work labels Feb 20, 2017

fix($http): JSON parse failure

573e0da

Fixes angular#15695

gkalpak suggested changes Feb 21, 2017

View reviewed changes

chirag64 added 2 commits February 22, 2017 01:19

Merge branch 'master' of https://github.com/angular/angular.js into j…

9d0c8b8

…son-parse-fix

fix($http): JSON parse failure

7deaae9

Fixes angular#15695

Merge branch 'master' of https://github.com/angular/angular.js into j…

28cc76c

…son-parse-fix

fix($http): JSON parse failure

91393a2

Fixes angular#15695

gkalpak reviewed Mar 4, 2017

View reviewed changes

Narretz modified the milestones: Backlog, 1.6.4 Mar 8, 2017

gkalpak approved these changes Mar 13, 2017

View reviewed changes

gkalpak closed this in c80fa1c Mar 14, 2017

gkalpak pushed a commit that referenced this pull request Mar 14, 2017

fix($http): throw more informative error on invalid JSON response

df88873

Fixes #15695 Closes #15724

ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017

fix($http): throw more informative error on invalid JSON response

efdd5a1

Fixes angular#15695 Closes angular#15724

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($http): JSON parse failure #15724

fix($http): JSON parse failure #15724

chirag64 commented Feb 19, 2017 •

edited

Loading

googlebot commented Feb 19, 2017

googlebot commented Feb 19, 2017

chirag64 commented Feb 19, 2017

googlebot commented Feb 19, 2017

googlebot commented Feb 19, 2017

gkalpak Feb 20, 2017

gkalpak Feb 20, 2017

gkalpak Feb 20, 2017

gkalpak Feb 20, 2017

gkalpak Feb 20, 2017

gkalpak Feb 20, 2017

gkalpak commented Feb 20, 2017

Narretz commented Feb 20, 2017

gkalpak Feb 21, 2017

gkalpak Feb 21, 2017

gkalpak Feb 21, 2017

chirag64 Feb 21, 2017

gkalpak Feb 21, 2017

gkalpak Feb 21, 2017

gkalpak Feb 21, 2017

Narretz commented Feb 22, 2017

gkalpak commented Feb 22, 2017

chirag64 commented Mar 2, 2017

Narretz commented Mar 2, 2017

gkalpak commented Mar 3, 2017

Narretz commented Mar 3, 2017

chirag64 commented Mar 4, 2017

gkalpak Mar 4, 2017

chirag64 Mar 8, 2017

Narretz Mar 11, 2017

gkalpak Mar 12, 2017

chirag64 Mar 12, 2017

gkalpak commented Mar 13, 2017

Narretz commented Mar 14, 2017


		To resolve this error, make sure you pass a valid JSON data object to `transformResponse`.

		For more information, see the {@link ng$http#default-transformations `transformResponse`} service


		The error message should provide additional context such as the actual response.

		To resolve this error, make sure you pass a valid JSON data object to `transformResponse`.

fix($http): JSON parse failure #15724

fix($http): JSON parse failure #15724

Conversation

chirag64 commented Feb 19, 2017 • edited Loading

googlebot commented Feb 19, 2017

googlebot commented Feb 19, 2017

chirag64 commented Feb 19, 2017

googlebot commented Feb 19, 2017

googlebot commented Feb 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Feb 20, 2017

Narretz commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Feb 22, 2017

gkalpak commented Feb 22, 2017

chirag64 commented Mar 2, 2017

Narretz commented Mar 2, 2017

gkalpak commented Mar 3, 2017

Narretz commented Mar 3, 2017

chirag64 commented Mar 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Mar 13, 2017

Narretz commented Mar 14, 2017

chirag64 commented Feb 19, 2017 •

edited

Loading